trade execute: bind EVM receipt confirmation to the locally-derived tx hash - #519
Conversation
trade execute confirmed a broadcast transaction by polling waitForReceipt with whatever txHash the broadcaster reported, without checking it against the transaction the CLI actually signed. A compromised or buggy broadcaster could report success for a different transaction, and the CLI would proceed to the next step (e.g. broadcasting a swap after a "confirmed" allowance revoke that never happened on-chain). Add evmTxHash (keccak256 of the signed tx bytes) and confirmEvmBroadcast, which polls the receipt on our own locally-derived hash and fails closed if the broadcaster's reported hash disagrees. Apply it at every EVM executeTransaction/waitForReceipt pair in trade execute (swap, approval, and revoke-then-reapprove, across the Privy/WalletConnect/local-key paths), with a carve-out for gasless swaps where the Relay solver broadcasts its own transaction and the returned hash is legitimately different. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pr-reviewer Summary for #64088d8✅ No issues found The code review completed successfully with no findings. Review effort: 4/5 (Complex) SummaryThis PR closes a real security gap: previously, Overall assessment: The implementation is sound. A few things were verified to be non-issues:
Token usage: 8,466 input, 4,646 output, 907,504 cache read, 56,522 cache write | Usage Guide New pushes are reviewed automatically with a 10-minute cooldown between reviews. To request a review at any time, comment |
…path Fold the duplicated TXHASH_MISMATCH check (confirmEvmBroadcast plus the two inline WalletConnect broadcast sites) into a single assertTxHashMatch helper that returns the locally-derived hash and fails closed on a mismatch. No behavior change — the WalletConnect sites already bound their receipt wait to the local hash; this just removes the copy-pasted error message. Add a unit test for guarantee #2: when the broadcaster returns no hash, confirmEvmBroadcast polls OUR locally-derived hash, never a foreign hash that happens to have a receipt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses pr-reviewer's finding on #519: the four confirmEvmBroadcast success-log lines printed the broadcaster's reported txHash, while the receipt was actually polled on our locally-derived hash. They are identical whenever the equality check passes (always, today), so the log was never wrong — but it showed a value we hadn't independently verified. Return the local hash from confirmEvmBroadcast alongside the receipt and log that instead, so the success line always names the transaction we confirmed landed. Extend the guarantee-#2 test to assert the returned hash is our local hash. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The main EVM swap block dropped its `&& result.txHash` guard so the non-gasless path can fall back to polling our locally-derived hash (guarantee #2). But the gasless branch has no local hash to bind to — the Relay solver broadcasts its own tx — so a gasless success with no reported hash would call waitForReceipt(chain, undefined), poll eth_getTransactionReceipt([undefined]) for the full 180s timeout, and then falsely report a revert. Restore the pre-existing skip for the gasless branch only: poll a receipt when the solver reports a hash, otherwise skip. Non-gasless behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The swap-path and allowance-path confirmEvmBroadcast/assertTxHashMatch sites all rethrow TXHASH_MISMATCH, but every one of those rethrows propagated to the quote loop's catch (quoteErr), which logged "Quote failed" and moved on to the next quote (or ended in ALL_QUOTES_FAILED). So a broadcaster-integrity failure was still swallowed and treated like a bad quote, contradicting the guarantee that it fails closed. Rethrow TXHASH_MISMATCH out of the quote-retry catch so it aborts the whole execute instead of falling through to the next quote. The prior single-quote test passed against the swallow bug because its message assertion also matched the ALL_QUOTES_FAILED wrapper. Strengthen it: a second quote (so a swallow would visibly retry), assert the thrown code is TXHASH_MISMATCH, assert only one /execute body was sent, and assert no "Trying next quote" log. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses pr-reviewer's finding on #519: evmTxHash(signedTransaction) was called inside the receipt-poll try whose catch prints "Transaction was broadcast but REVERTED on-chain!". A signedTransaction that is not valid hex throws before any poll runs, so that near-impossible path would show a revert diagnostic for a transaction that was never polled. Derive the local hash in a guarded pre-step outside the poll try: a hex-validation failure now surfaces as INVALID_SIGNED_TX with its real message, and the revert catch only ever fires for an actual poll. Also drops the redundant in-try evmTxHash call (confirmEvmBroadcast recomputes it) and keeps the single shared revert/next-quote handler for both the gasless and non-gasless branches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@nansen-pr-reviewer re-review |
![hulk-linus[bot]](https://avatars.githubusercontent.com/in/4537018?s=60&v=4){kind=small}
There was a problem hiding this comment.
Retracted — this review was broader than the PR scope and is superseded by the scoped exact-head review: #519 (review)
Only F1 and F4 are being requested for this PR. F2 and F5 are follow-ups; F3 is withdrawn.
Codier
left a comment
Codier
left a comment
There was a problem hiding this comment.
I took another pass on scope. I think only these two findings should be addressed in this PR.
F1 — Treat every post-broadcast local-hash failure as fatal
At src/trading.js:3162-3164, INVALID_SIGNED_TX can be raised after /execute has already returned Success, but the quote boundary at :3247 only treats TXHASH_MISMATCH as fatal. The approval/revoke paths have the same shape: local hash derivation happens after a successful broadcast, while their catches only rethrow the mismatch code.
At that point the broadcast outcome is uncertain. Trying the next quote can submit another transaction, which is exactly the behavior this PR is trying to prevent.
Please either derive/validate the local hash before calling /execute, or make every local-hash derivation failure after a successful response fatal across the swap, approval, and revoke paths. Add a two-quote regression proving there is only one /execute call and the original error reaches the user.
F4 — A receipt timeout is not a confirmed revert and must not retry
waitForReceipt distinguishes a confirmed on-chain revert from “receipt not found after 180000ms”, but the catch at src/trading.js:3178-3187 renders both as REVERTED on-chain and tries the next quote.
This directly weakens the PR's new no-broadcaster-hash guarantee: a silent substitution is supposed to time out while polling our local hash, but that timeout currently causes another swap to be broadcast. A legitimately pending transaction has the same duplicate-broadcast risk.
Please distinguish confirmed revert from timeout/unconfirmed receipt. A confirmed revert may follow the existing retry behavior; a timeout or RPC-unconfirmed result is uncertain post-broadcast state and should abort. Add a two-quote regression proving the timeout path makes only one /execute call and never logs “Trying next quote”.
Exact-head review of e536da7ff18b2a5f765274fec8b09d3c2f8b5a87 · review-only · OpenAI GPT-5.6 Sol
|
Closing loops on F1 and F4 findings from the GPT-5.6 Sol review. F1: fatal local-hash derivation failuresYou addressed this for the main swap path by moving However, the approval and revoke paths in both the Privy and local-key signers (e.g., Please ensure that F4: timeout is not a revertIn A timeout (uncertain state) must not retry. Please update I've verified the current tests pass locally, but we need these specific edge cases covered and enforced to close the trust boundary. |
Addresses the GPT-5.6 Sol review and Codier's follow-up on #519. Once a transaction is broadcast, "try the next quote" can broadcast a second transaction while the first's fate is unknown — the failure class this PR exists to close. Three gaps remained where the CLI still moved past an unverified broadcast: - INVALID_SIGNED_TX (F1): a local-hash derivation failure after a successful broadcast was only fatal on the main swap path. Route it through assertTxHashMatch as a coded error and treat it as fatal at every swap/approval/revoke catch (Privy, WalletConnect, local-key) via a shared isFatalBroadcastError helper. - RECEIPT_TIMEOUT (F4): waitForReceipt threw an untyped Error on timeout, which every caller rendered as "REVERTED on-chain" and retried. A timeout is uncertain, not a confirmed revert — retrying races a second tx against the same nonce. Tag the timeout with a code and fail closed on it everywhere, keeping the retry only for a genuine on-chain revert. - TXHASH_MISMATCH prefix sensitivity (F3): the comparison was 0x-prefix-sensitive while evmTxHash always emits 0x-prefixed, so a bare-hex broadcaster hash was a false (now-fatal) mismatch. Normalize both sides before comparing. Also forward the label through confirmEvmBroadcast so a Privy/local-key revoke mismatch names itself — the least useful moment to lose context. Tests: bare-hex no-false-mismatch, RECEIPT_TIMEOUT tagging, and two-quote regressions proving the swap-timeout, revoke-timeout, and INVALID_SIGNED_TX paths each make exactly one /execute call and never try the next quote. Verified end-to-end against a local mock via the real `trade execute` binary: a mismatched broadcaster hash aborts with TXHASH_MISMATCH, and a never-landing receipt aborts with RECEIPT_TIMEOUT — both before any success is reported. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codier
left a comment
Codier
left a comment
There was a problem hiding this comment.
Looks good. You've correctly ensured that any post-broadcast uncertainty — whether it's a hash mismatch, a derivation failure, or a receipt timeout — is treated as fatal across all paths (swap, approval, revoke). This closes the double-broadcast risk.
CI is green and the new test coverage for these edge cases is solid.
Approving.
AI execution metadata
- Provider: google
- Model:
gemini-3-flash-preview - Thinking level: high
- Attribution: exact-head review of
64088d81a5ed18161ae349feec18aaf53c51e531
2 participants
Summary
trade executeconfirmed a broadcast EVM transaction by pollingwaitForReceiptwith whatevertxHashthe broadcaster reported, without ever checking it against the transaction the CLI actually signed. A compromised or buggy broadcaster could report success for a different transaction, and the CLI would proceed to the next step (e.g. broadcasting a swap after a "confirmed" allowance revoke that never happened on-chain).evmTxHash(keccak256 over the raw signed tx bytes) andconfirmEvmBroadcast, which polls the receipt on our own locally-derived hash and fails closed with a clear, actionable error if the broadcaster's reported hash disagrees.executeTransaction/waitForReceiptpair intrade execute— the swap broadcast, ordinary approvals, and the revoke-then-reapprove flow — across the Privy, WalletConnect, and local-key signing paths.TXHASH_MISMATCHis now fatal (re-thrown) everywhere it can occur, instead of being swallowed into "try the next quote" — a broadcaster-integrity failure shouldn't be treated like a bad quote.src/bridge.jsunchanged (out of scope — no raw signed bytes available in the former; different broadcast mechanism in the latter).Test plan
npm test— 2127 tests passing, including newevmTxHashunit tests (known-vector, hex normalization, rejection cases, EIP-1559/legacy signing round-trips), a fail-closed mismatch test, and a gasless-regression testnpm run lintexecutetest mocks updated to echo the real hash of the signed bytes, as a correct broadcaster wouldnode src/index.js trade quote/trade execute) against a local mock trading-api + RPC server: a correct broadcaster confirms normally, a broadcaster returning a mismatched hash is rejected with the fail-closed error, before any success is reported🤖 Generated with Claude Code